feat(useExplicitType): support explicit function argument types #4463
Conversation
github-actions
bot
added
A-CLI
kaykdm
force-pushed
the
explicit-argument-types
branch
from
45fa76f
to
925a373
Compare
CodSpeed Performance ReportMerging #4463 will degrade performances by 9.81%Comparing Summary
Benchmarks breakdown
|
| } | ||
| } | ||
|
|
||
| declare_node_union! { | ||
| pub AnyJsFunctionWithReturnType = AnyJsFunction | JsMethodClassMember | JsMethodObjectMember | JsGetterClassMember | JsGetterObjectMember | ||
| pub AnyJsFunctionWithReturnTypeOrJsParameters = AnyJsFunction | JsMethodClassMember | JsMethodObjectMember | JsGetterClassMember | JsGetterObjectMember | JsParameters |
There was a problem hiding this comment.
I've just noticed that some types are missing for checking if the return type is explicitly set:
TsMethodSignatureTypeMemberTsCallSignatureTypeMemberTsMethodSignatureClassMemberTsSetterSignatureClassMemberTsDeclareFunctionDeclarationTsDeclareFunctionExportDefaultDeclaration
I wonder if directly matching against JsParameters is a good idea. I see two possible alternative approaches:
- matching directly against
JsFormalParameter,JsRestParameter, andTsThisParameter - matching against functions with parameters (We already have most of them, we need to add setter and constructors).
Personally I could choose (2) because this allows us to issue a single diagnostic when a parameter or a return type is absent on a function.
Also, similarly to return type, we should ignore callbacks.
Besides, we should think to a better name for this union because we will have to add properties and top-level variables. Maybe AnyJsTypeableEntity? Have you a better suggestion?
There was a problem hiding this comment.
Thanks for the review, and apologies for the late response.
I've just noticed that some types are missing for checking if the return type is explicitly set:
I understand. I'll create a separate for this this.
I wonder if directly matching against JsParameters is a good idea. I see two possible alternative approaches:
I see your point. I'll revisit this after implementing the missing return types.
Maybe AnyJsTypeableEntity? Do you have a better suggestion?
AnyJsTypeableEntity looks good to me.
For now, I will make this PR draft.
|
|
||
| enum UseExplicitTypeCause { | ||
| MissingReturnType, | ||
| MissingArgumentnType(String), |
There was a problem hiding this comment.
Usually we try to avoid allocating a string. You could directly take the token:
| MissingArgumentnType(String), | |
| MissingArgumentnType(JsSyntaxToken), |
In this case we could rewrite UseExplicitTypeState as:
pub enum UseExplicitTypeState {
MissingReturnType(TextRange),
MissingArgumentnType(JsSyntaxToken),
}Because we can get the range from a token.
| for p in parameters.items() { | ||
| let param = p.ok()?; |
There was a problem hiding this comment.
We could avoid returning as soon as we met an error by iterating only over node without errors:
| for p in parameters.items() { | |
| let param = p.ok()?; | |
| for p in parameters.items().into_iter().flatten() { |
| fn check_function_parameters_type(parameters: &JsParameters) -> Option<UseExplicitTypeState> { | ||
| for p in parameters.items() { | ||
| let param = p.ok()?; | ||
| let formal_param = param.as_any_js_formal_parameter()?; |
There was a problem hiding this comment.
We should certainly handle the other parameter kinds (rest parameter and this parameter).
Summary
related: #2017
This PR adds support for enforcing explicit type annotations on arguments in all functions and class methods. The rule is inspired by the explicit-module-boundary-types rule, but it expands coverage beyond exported functions to include all functions and methods within a class.